Add check for `[T;N]`/`usize` mismatch in astconv #80538

JulianKnodt · 2020-12-30T23:26:44Z

Helps clarify the issue in #80506
by adding a specific check for mismatches between [T;N] and usize.

rust-log-analyzer · 2020-12-30T23:39:59Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/compiler/rustc_typeck/src/astconv/generics.rs at line 7:
 use rustc_errors::{pluralize, struct_span_err, Applicability, DiagnosticId, ErrorReported};
 use rustc_hir as hir;
 use rustc_hir::def_id::DefId;
-use rustc_hir::{GenericArg};
+use rustc_hir::GenericArg;
 use rustc_middle::ty::{
     self, subst, subst::SubstsRef, GenericParamDef, GenericParamDefKind, Ty, TyCtxt,
 };
Diff in /checkout/compiler/rustc_typeck/src/astconv/generics.rs at line 61:
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_typeck/src/astconv/generics.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
 
         // Specific suggestion set for diagnostics
         if let Some(param) = param {
-          match (arg, &param.kind) {
-              (
-                  GenericArg::Type(hir::Ty { kind: hir::TyKind::Path { .. }, .. }),
-                  GenericParamDefKind::Const { .. },
-              ) => {
-                  let suggestions = vec![
-                      (arg.span().shrink_to_lo(), String::from("{ ")),
-                      (arg.span().shrink_to_hi(), String::from(" }")),
-                  ];
-                  err.multipart_suggestion(
-                      "if this generic argument was intended as a const parameter, \
+            match (arg, &param.kind) {
+                (
+                    GenericArg::Type(hir::Ty { kind: hir::TyKind::Path { .. }, .. }),
+                    GenericParamDefKind::Const { .. },
+                ) => {
+                    let suggestions = vec![
+                        (arg.span().shrink_to_lo(), String::from("{ ")),
+                        (arg.span().shrink_to_hi(), String::from(" }")),
+                    ];
+                    err.multipart_suggestion(
+                        "if this generic argument was intended as a const parameter, \
                     try surrounding it with braces:",
-                      suggestions,
-                      Applicability::MaybeIncorrect,
-                  );
-              (
-              (
-                  GenericArg::Type(hir::Ty { kind: hir::TyKind::Array(_, len), .. }),
-                  GenericParamDefKind::Const { .. },
-              ) => if tcx.type_of(param.def_id).is_ptr_sized_integral() {
-                  let snippet = sess
-                      .source_map()
-                      .span_to_snippet(tcx.hir().span(len.hir_id))
-                      .unwrap_or(String::from("N"));
-                  err.span_suggestion(
-                      arg.span(),
-                      "`[T; N]` provided where `usize` was expected, try",
-                      format!("{}", snippet),
-                      Applicability::MaybeIncorrect,
-                  );
-              _ => {}
-          }
+                        suggestions,
+                        suggestions,
+                        Applicability::MaybeIncorrect,
+                    );
+                (
+                (
+                    GenericArg::Type(hir::Ty { kind: hir::TyKind::Array(_, len), .. }),
+                    GenericParamDefKind::Const { .. },
+                ) => {
+                    if tcx.type_of(param.def_id).is_ptr_sized_integral() {
+                        let snippet = sess
+                            .source_map()
+                            .span_to_snippet(tcx.hir().span(len.hir_id))
+                            .unwrap_or(String::from("N"));
+                        err.span_suggestion(
+                            arg.span(),
+                            "`[T; N]` provided where `usize` was expected, try",
+                            format!("{}", snippet),
+                            Applicability::MaybeIncorrect,
+                        );
+                }
+                _ => {}
+            }
         }
         }
 
         // This note is only true when generic parameters are strictly ordered by their kind.
Build completed unsuccessfully in 0:00:14

lcnr

thanks ❤️

can you also add a test for [u8; 1 + 2]? We probably want to always recommend using braces for now as especially for complex expressions the error recovery when they are missing is fairly hard.

compiler/rustc_typeck/src/astconv/generics.rs

compiler/rustc_middle/src/ty/mod.rs

rust-log-analyzer · 2021-01-04T09:55:37Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking rustc_attr v0.0.0 (/checkout/compiler/rustc_attr)
    Checking rustc_hir v0.0.0 (/checkout/compiler/rustc_hir)
    Checking rustc_parse v0.0.0 (/checkout/compiler/rustc_parse)
    Checking chalk-engine v0.36.0
error[E0599]: no method named `features` found for reference `&Session` in the current scope
   --> compiler/rustc_hir/src/hir.rs:299:60
    |
299 |                 ast::ParamKindOrd::Const { unordered: sess.features().const_generics }
    |                                                            ^^^^^^^^ private field, not a method
error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
error: could not compile `rustc_hir`
error: could not compile `rustc_hir`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" " llvm max_level_info" "--manifest-path" "/checkout/compiler/rustc/Cargo.toml" "-p" "rustc-main" "-p" "rustc_codegen_ssa" "-p" "rustc_session" "-p" "rustc_lint_defs" "-p" "rustc_feature" "-p" "rustc_data_structures" "-p" "rustc_graphviz" "-p" "rustc_target" "-p" "rustc_index" "-p" "rustc_span" "-p" "rustc_arena" "-p" "rustc_macros" "-p" "rustc_symbol_mangling" "-p" "rustc_middle" "-p" "rustc_query_system" "-p" "rustc_type_ir" "-p" "rustc_incremental" "-p" "rustc_apfloat" "-p" "rustc_hir" "-p" "rustc_ast" "-p" "rustc_lexer" "-p" "rustc_errors" "-p" "rustc_attr" "-p" "rustc_ast_pretty" "-p" "rustc_fs_util" "-p" "rustc_serialize" "-p" "rustc_driver" "-p" "rustc_parse" "-p" "rustc_error_codes" "-p" "rustc_lint" "-p" "rustc_trait_selection" "-p" "rustc_infer" "-p" "rustc_parse_format" "-p" "rustc_mir" "-p" "coverage_test_macros" "-p" "rustc_hir_pretty" "-p" "rustc_metadata" "-p" "rustc_expand" "-p" "rustc_ast_passes" "-p" "rustc_interface" "-p" "rustc_builtin_macros" "-p" "rustc_mir_build" "-p" "rustc_resolve" "-p" "rustc_codegen_llvm" "-p" "rustc_llvm" "-p" "rustc_ty_utils" "-p" "rustc_traits" "-p" "rustc_passes" "-p" "rustc_ast_lowering" "-p" "rustc_privacy" "-p" "rustc_typeck" "-p" "rustc_save_analysis" "-p" "rustc_plugin_impl" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:50

lcnr · 2021-01-04T11:55:56Z

thanks 👍

@bors r+ rollup

bors · 2021-01-04T11:55:58Z

📌 Commit 54883e0 has been approved by lcnr

Rollup of 12 pull requests Successful merges: - rust-lang#80442 (Mention Arc::make_mut and Rc::make_mut in the documentation of Cow) - rust-lang#80533 (bootstrap: clippy fixes) - rust-lang#80538 (Add check for `[T;N]`/`usize` mismatch in astconv) - rust-lang#80612 (Remove reverted change from relnotes) - rust-lang#80627 (Builder: Warn if test file does not exist) - rust-lang#80637 (Use Option::filter instead of open-coding it) - rust-lang#80643 (Move variable into the only branch where it is relevant) - rust-lang#80656 (Fixed documentation error for `std::hint::spin_loop`) - rust-lang#80666 (Fix missing link for "fully qualified syntax") - rust-lang#80672 (./x.py clippy: allow the most noisy lints) - rust-lang#80677 (doc -- list edit for consistency) - rust-lang#80696 (make sure that promoteds which fail to evaluate in dead const code behave correctly) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

This reverts commit be2a3f8, reversing changes made to 4c4e8e7.

rylev · 2021-01-08T09:51:23Z

@JulianKnodt @lcnr it seems this change was responsible for a moderate performance regression (and a minor perf gain). You can see the results in #80798 where we reverted this PR and ran the perf suite.

The regression was in the deeply-nested-async benchmark which does not contain any const code it seems. Any ideas?

lcnr · 2021-01-08T10:04:58Z

compiler/rustc_typeck/src/astconv/generics.rs

                                // argument. That means we're inferring the lifetimes.
                                substs.push(ctx.inferred_kind(None, param, infer_args));
-                                force_infer_lt = Some(arg);
+                                force_infer_lt = Some((arg, param));


is it possible that this loop is a lot hotter than I expected so storing arg with param here is quite costly?

Other than that i really don't know waht could be causing the perf regression.

I agree that this would be the most likely cause, it could be that the type for param for deeply-nested-async stuff is huge and thus takes up a lot of space on the stack, so even if it's not hot it might just be causing a lot more cache misses

edit: these are refs, so the size shouldn't matter, ignore above

Would it be good to change this to an index instead? I don't remember where the param was coming from at the moment

It seems possible that this increases the size of force_infer_lt enough that it becomes stored on the stack instead of a register, I guess, but it's pretty odd that it would cause any problems (still just two pointers vs one). An index would still be a usize so should be equally costly I'd expect.

I added a print to the inner body, and during compilation it appears that the body is hit at least 1.5M times, so I guess that's fairly hot?

you mean during the compilation of the compiler itself? so during ./x.py build --stage 2 or just while building std?

I was running ./x.py test src/test/ui, but it got to compiling stage 1 artifacts and I quit out

rust-highfive assigned lcnr Dec 30, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 30, 2020

JulianKnodt force-pushed the err_usize branch from 673227d to 2445da2 Compare December 30, 2020 23:28

JulianKnodt force-pushed the err_usize branch from 2445da2 to 68fa1ac Compare December 30, 2020 23:47

lcnr reviewed Dec 31, 2020

View reviewed changes

compiler/rustc_typeck/src/astconv/generics.rs Outdated Show resolved Hide resolved

compiler/rustc_typeck/src/astconv/generics.rs Outdated Show resolved Hide resolved

compiler/rustc_typeck/src/astconv/generics.rs Outdated Show resolved Hide resolved

JulianKnodt force-pushed the err_usize branch from 68fa1ac to a544286 Compare December 31, 2020 21:40

lcnr reviewed Jan 1, 2021

View reviewed changes

compiler/rustc_typeck/src/astconv/generics.rs Outdated Show resolved Hide resolved

JulianKnodt force-pushed the err_usize branch from a544286 to 521515e Compare January 1, 2021 21:48

lcnr reviewed Jan 2, 2021

View reviewed changes

compiler/rustc_typeck/src/astconv/generics.rs Outdated Show resolved Hide resolved

JulianKnodt force-pushed the err_usize branch 2 times, most recently from 4885728 to 029ff3f Compare January 4, 2021 01:14

lcnr reviewed Jan 4, 2021

View reviewed changes

compiler/rustc_middle/src/ty/mod.rs Outdated Show resolved Hide resolved

JulianKnodt force-pushed the err_usize branch from 029ff3f to 4767cf0 Compare January 4, 2021 09:52

JulianKnodt force-pushed the err_usize branch 2 times, most recently from ad6ad28 to 7091cf7 Compare January 4, 2021 10:05

Add check for array/usize mismatch in astconv

54883e0

JulianKnodt force-pushed the err_usize branch from 7091cf7 to 54883e0 Compare January 4, 2021 10:07

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2021

JohnTitor mentioned this pull request Jan 5, 2021

Rollup of 12 pull requests #80708

Merged

bors merged commit be2a3f8 into rust-lang:master Jan 5, 2021

rustbot added this to the 1.51.0 milestone Jan 5, 2021

JulianKnodt deleted the err_usize branch January 5, 2021 06:05

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 7, 2021

Revert "Rollup merge of rust-lang#80538 - JulianKnodt:err_usize, r=lcnr"

9ac4831

This reverts commit be2a3f8, reversing changes made to 4c4e8e7.

lcnr reviewed Jan 8, 2021

View reviewed changes

JulianKnodt mentioned this pull request Jan 8, 2021

Clean create_substs_for_generic_args #80827

Closed

lcnr added the A-const-generics Area: const generics (parameters and arguments) label Dec 11, 2021

lcnr mentioned this pull request Dec 11, 2021

Tracking issue for const generics (RFC 2000) #44580

Closed

16 tasks

Add check for [T;N]/usize mismatch in astconv #80538

Add check for [T;N]/usize mismatch in astconv #80538

Uh oh!

Conversation

JulianKnodt commented Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-log-analyzer commented Dec 30, 2020

Uh oh!

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rust-log-analyzer commented Jan 4, 2021

Uh oh!

lcnr commented Jan 4, 2021

Uh oh!

bors commented Jan 4, 2021

Uh oh!

rylev commented Jan 8, 2021

Uh oh!

lcnr Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

JulianKnodt Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

JulianKnodt Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

lcnr Jan 9, 2021

Choose a reason for hiding this comment

Uh oh!

JulianKnodt Jan 9, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Add check for `[T;N]`/`usize` mismatch in astconv #80538

Add check for `[T;N]`/`usize` mismatch in astconv #80538

JulianKnodt commented Dec 30, 2020 •

edited

Loading

JulianKnodt Jan 8, 2021 •

edited

Loading